-
Notifications
You must be signed in to change notification settings - Fork 574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
many: send components information on GET /v2/snaps #14060
many: send components information on GET /v2/snaps #14060
Conversation
ea3afc4
to
5b2a83e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a couple nitpicks
// when its symlink was created. We cannot use the mount directory as lstat | ||
// returns the date of the root of the container instead of the date when the | ||
// mount directory was created. | ||
func ComponentInstallDate(cpi ContainerPlaceInfo, snapRev Revision) *time.Time { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this follow the same pattern as snap.InstallDate
? It returns a time.Time{}
when we can't figure out the time, rather than a nil pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I see that you're using a pointer inside of client.Component
, so it works well with omitempty
. So this is probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But Info.InstallDate()
returns *time.Time
so we are already inconsistent. I think that omitempty would work fine also if we return 0 though.
snap/component.go
Outdated
} | ||
|
||
// ComponentSize returns the file size of a component. | ||
func ComponentSize(cpi ContainerPlaceInfo) int64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also feels like it should return an error here if something goes wrong, and make the caller decide if it wants to default to zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewphelpsj thanks for your review, I've addressed your comments now
// when its symlink was created. We cannot use the mount directory as lstat | ||
// returns the date of the root of the container instead of the date when the | ||
// mount directory was created. | ||
func ComponentInstallDate(cpi ContainerPlaceInfo, snapRev Revision) *time.Time { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But Info.InstallDate()
returns *time.Time
so we are already inconsistent. I think that omitempty would work fine also if we return 0 though.
snap/component.go
Outdated
} | ||
|
||
// ComponentSize returns the file size of a component. | ||
func ComponentSize(cpi ContainerPlaceInfo) int64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes look good in themselves, but I think we need to think how to expose also the information about defined but not installed components, the spec requires to print #installed/#defined
iirc. We might have to extend select to take a comma separated list of selectors or introduce select-components?
client/packages.go
Outdated
@@ -91,6 +91,8 @@ type Snap struct { | |||
GatingHold *time.Time `json:"gating-hold,omitempty"` | |||
// if RefreshInhibit is nil, then there is no pending refresh. | |||
RefreshInhibit *SnapRefreshInhibit `json:"refresh-inhibit,omitempty"` | |||
|
|||
Comps []Component `json:"components,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call this Components, we don't seem to use .Comps anywhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
That is a good point. I think that an reasonable solution would be that the list of returned components in a GET could also include non-installed components, and we can easily differentiate between both cases as the non-installed won't have `install-date/installed-size" fields. |
@pedronis I've changed this now so the not installed components are sent as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you
a0d59fd
to
2c3bc45
Compare
Commits squashed now |
Send back components information on GET /v2/snaps and GET /v2/snaps/{name} requests.